Use asyncio - #2880
Conversation
Add a test that checks that "tcp server mode" works. Server meaning that this plugin acts as the TCP server and the langserver connects as TCP client.
Add a test that checks that "tcp server mode" works. Server meaning that this plugin acts as the TCP server and the langserver connects as TCP client.
Conflicts: tests/server.py
✅ Deploy Preview for sublime-lsp ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Also write the rest of the requests in terms of Session.request
|
Would be useful to split it into smaller chunks, if possible. For example it would likely be possible to make Lots of assumptions on my side but that's what I feel. I was actually looking into that before as I wanted to move start_async into dedicated thread so that it doesn't black other plugins (kinda opposite goal of yours but also kinda similar as I guess with asyncio it will also run on dedicated thread). See #2863 It will be hard to review it properly with a big dump of code that refactors most of the code base. |
- Add sublime.set_timeout executor wrapper - Make all request handlers `async` - Define a CancellableInflightStreamingRequest class that enables `async for` syntax - Start inheriting DocumentSyncListener from sublime_aio.ViewEventListener (This one doesn't work yet) The state is fairly broken at this point.
Co-authored-by: Rafał Chłodnicki <rchl2k@gmail.com>
There was a problem hiding this comment.
I have this reproducible crash that ends up breaking other stuff.
- I'm working on python files on which I have basedpyright and ruff running.
- I open my test
HelloWorld.javafile which triggers initialization ofLSP-jdtls - For one reason or another jdtls fails to start on my system. LSP prints to the console:
LSP: jdtls crashed (1 / 5 times in the last 180.0 seconds), exit code 13, exception: None
- Now on opening new python files basedpyright and ruff don't start on those files anymore.
The example java file itself has this code:
public class HelloWorld {
// Your program begins with a call to main()
public static void main(String[] args) {
// Prints "Hello, World" to the terminal window.
System.out.println("Hello, World");
}
}On main it tries to start 5 times and each time prints more useful error message (server's output):
LSP: jdtls crashed (1 / 5 times in the last 180.0 seconds), exit code 0, exception: Unexpected payload in server's stdout:
An error has occurred. See the log file
/Users/rafal/Library/Caches/Sublime Text/Package Storage/LSP-jdtls/data/.metadata/.log.
LSP: jdtls crashed (2 / 5 times in the last 180.0 seconds), exit code 0, exception: Unexpected payload in server's stdout:
An error has occurred. See the log file
/Users/rafal/Library/Caches/Sublime Text/Package Storage/LSP-jdtls/data/.metadata/.log.
...
| """Holds state per request.""" | ||
|
|
||
| def __init__(self, sv: SessionViewProtocol, request_id: int, request: Request[Any, Any]) -> None: | ||
| def __init__(self, sv: SessionViewProtocol, cancellable: RequestController, request: Request[Any, Any]) -> None: |
There was a problem hiding this comment.
Let's rename cancellable to controller
|
|
||
|
|
||
| def run_coroutine( | ||
| coroutine: Coroutine[object, object, T], *, exception_policy: ExceptionPolicy = ExceptionPolicy.STACKTRACE |
There was a problem hiding this comment.
Seems like exception_policy is never used so why introduce it now?
Even if we think of potential use for it later, I don't think that ExceptionPolicy.MESSAGEBOX would be that useful. More likely someone would want to show some custom error rather than just dump the exception in the message box.
| return _run_on_st_thread(sublime.set_timeout_async, f, *args, **kwargs) | ||
|
|
||
|
|
||
| def tick(n: int = 1) -> asyncio.Future[None]: |
There was a problem hiding this comment.
I have hard time thinking of a use case for being able to specify the exact number of ticks. Can we remove that (not currently used) feature?
| flattened list of Exceptions that occurred for each coroutine. BaseExceptions are filtered out. | ||
| """ | ||
| exceptions: list[Exception] = [] | ||
| items: list[BaseException | list[Exception]] = await asyncio.gather(*coros, return_exceptions=True) |
There was a problem hiding this comment.
Redundant annotation
| items: list[BaseException | list[Exception]] = await asyncio.gather(*coros, return_exceptions=True) | |
| items = await asyncio.gather(*coros, return_exceptions=True) |
|
|
||
| def show_summary_message( | ||
| window: sublime.Window, result: ApplyWorkspaceEditResult, summary: WorkspaceEditSummary | ||
| window: sublime.Window, result: ApplyWorkspaceEditResult, summary: WorkspaceEditSummary | BaseException |
There was a problem hiding this comment.
Does it still hold that the summary can be a BaseException? At least the types at call sites suggest that it can't.
| view = await open_file(window, decoded_uri, flags, group) | ||
| if view: | ||
| return center_selection(view, r) | ||
| return None | ||
| if fragment := urlparse(decoded_uri).fragment: | ||
| if selection := lsp_range_from_uri_fragment(fragment): | ||
| center_selection(view, selection) | ||
| return view |
There was a problem hiding this comment.
less lines
if view and (fragment := urlparse(decoded_uri).fragment) and (selection := lsp_range_from_uri_fragment(fragment)):
center_selection(view, selection)| def on_main_thread() -> None: | ||
|
|
||
| # window.open_file brings the file to focus if it's already opened, which we don't want (unless it's | ||
| # supposed to open as a separate view). | ||
| view = _find_open_file(window, file) | ||
| if view and _return_existing_view(flags, window.get_view_index(view)[0], window.active_group(), group): | ||
| loop.call_soon_threadsafe(lambda: resolve_right_now(view)) | ||
| return | ||
|
|
||
| was_already_open = view is not None | ||
| if not was_already_open and not os.path.isfile(file): | ||
| # window.open_file creates a new view with empty content if the path from the given URI doesn't | ||
| # exist as a file on disk, but we don't want that here. If the language server wants to create a new | ||
| # file for a given URI, it must use the CreateFile resource operation in a WorkspaceEdit. | ||
| loop.call_soon_threadsafe(lambda: resolve_right_now(view)) | ||
| return | ||
|
|
||
| view = window.open_file(file, flags, group) | ||
| if not view.is_loading(): | ||
| if was_already_open and (flags & sublime.NewFileFlags.SEMI_TRANSIENT): | ||
| # workaround bug https://github.com/sublimehq/sublime_text/issues/2411 where transient view | ||
| # might not get its view listeners initialized. | ||
| sublime_plugin.check_view_event_listeners(view) # type: ignore | ||
| # It's already loaded. Possibly already open in a tab. | ||
| loop.call_soon_threadsafe(lambda: resolve_right_now(view)) | ||
|
|
||
| loop.call_soon_threadsafe(resolve_later) |
There was a problem hiding this comment.
Our convention is not to have blank lines in function bodies:
| def on_main_thread() -> None: | |
| # window.open_file brings the file to focus if it's already opened, which we don't want (unless it's | |
| # supposed to open as a separate view). | |
| view = _find_open_file(window, file) | |
| if view and _return_existing_view(flags, window.get_view_index(view)[0], window.active_group(), group): | |
| loop.call_soon_threadsafe(lambda: resolve_right_now(view)) | |
| return | |
| was_already_open = view is not None | |
| if not was_already_open and not os.path.isfile(file): | |
| # window.open_file creates a new view with empty content if the path from the given URI doesn't | |
| # exist as a file on disk, but we don't want that here. If the language server wants to create a new | |
| # file for a given URI, it must use the CreateFile resource operation in a WorkspaceEdit. | |
| loop.call_soon_threadsafe(lambda: resolve_right_now(view)) | |
| return | |
| view = window.open_file(file, flags, group) | |
| if not view.is_loading(): | |
| if was_already_open and (flags & sublime.NewFileFlags.SEMI_TRANSIENT): | |
| # workaround bug https://github.com/sublimehq/sublime_text/issues/2411 where transient view | |
| # might not get its view listeners initialized. | |
| sublime_plugin.check_view_event_listeners(view) # type: ignore | |
| # It's already loaded. Possibly already open in a tab. | |
| loop.call_soon_threadsafe(lambda: resolve_right_now(view)) | |
| loop.call_soon_threadsafe(resolve_later) | |
| def on_main_thread() -> None: | |
| # window.open_file brings the file to focus if it's already opened, which we don't want (unless it's | |
| # supposed to open as a separate view). | |
| view = _find_open_file(window, file) | |
| if view and _return_existing_view(flags, window.get_view_index(view)[0], window.active_group(), group): | |
| loop.call_soon_threadsafe(lambda: resolve_right_now(view)) | |
| return | |
| was_already_open = view is not None | |
| if not was_already_open and not os.path.isfile(file): | |
| # window.open_file creates a new view with empty content if the path from the given URI doesn't | |
| # exist as a file on disk, but we don't want that here. If the language server wants to create a new | |
| # file for a given URI, it must use the CreateFile resource operation in a WorkspaceEdit. | |
| loop.call_soon_threadsafe(lambda: resolve_right_now(view)) | |
| return | |
| view = window.open_file(file, flags, group) | |
| if not view.is_loading(): | |
| if was_already_open and (flags & sublime.NewFileFlags.SEMI_TRANSIENT): | |
| # workaround bug https://github.com/sublimehq/sublime_text/issues/2411 where transient view | |
| # might not get its view listeners initialized. | |
| sublime_plugin.check_view_event_listeners(view) # type: ignore | |
| # It's already loaded. Possibly already open in a tab. | |
| loop.call_soon_threadsafe(lambda: resolve_right_now(view)) | |
| loop.call_soon_threadsafe(resolve_later) |
| # It's already loaded. Possibly already open in a tab. | ||
| loop.call_soon_threadsafe(lambda: resolve_right_now(view)) | ||
|
|
||
| loop.call_soon_threadsafe(resolve_later) |
| @deprecated("use SessionBuffer.request_code_actions instead") | ||
| def request_code_actions_async( |
There was a problem hiding this comment.
Is this actually used anywhere? I can't find any references. Not even in packages.
| @deprecated("use Session.run_code_action instead") | ||
| def run_code_action_async( |
This PR switches the codebase to using
async deffunctions andasyncio. The loop provider issublime_aio.close #2863.
should be merged (and released) at the same time as:
The main driver for doing this is to decrease the thread usage of this plugin from O(n) to O(1) threads, where
nis the number of language servers running. The secondary driver is syntax sugar.Why is this PR so large? Please read: What color is your function?
Self-contained bits:
sublime.set_timeout_asyncLSP.plugin.core.aio.call_soon_threadsafedef f() -> Promise[T]: ...async def f() -> T: ...Promise.then(lambda x: ...)x = await f()session.send_request_async(R(), lambda x: ...)x = await session.request(R())try ... except ResponseException:blockasync for partial_result in session.stream(R()):(caveat: only works forlist[...]-style responses)LSP.plugin.core.aio.run_coroutine_threadsafe(f())PromiseobjectsPromise.thenawait promisePromise.allasyncio.gathersublime.set_timeout_async(f, timeout_ms=1000)await asyncio.sleep(1)threading.Lock, or write very complicated queueing logicasyncio.Lockasyncfunction in aPromisePromise.wrap_taskfcallsgggfasync def f(): await g()async def f(): g()f, guaranteed called from asyncio threadaio.TaskContainer.create_task(g())def f(): g()f, any threaddef f(): aio.run_coroutine_threadsafe(g()), or useaio.TaskContainer.create_task_threadsafe(g())def f(): g()The Plan
Make "most" code run on the sublime_aio thread
Most code is doing bookkeeping. This type of code used to run on the Sublime "async" thread. It should run on the asyncio loop thread.
Previously, the code attempted to make most code run on the ST async thread. We never really enforced this. We tried to make it clear that a function/method should be running on the ST async thread by suffixing it with
_async.If you have an
async defcoroutine function, then such a coroutine function is forced to run on the asyncio loop thread. So enforcement becomes automatic.Keep
_asyncsuffixes, assume they run on the asyncio threadWhen a method or function has the suffix
_asyncin its name, we tried to ensure these functions run on the ST async thread. These can now be assumed to be running on the asyncio thread.Make compute-intensive function run on the Sublime "async" thread
The only compute-intensive code we deal with are parsing and emitting JSON. Only the JSON parser/emitter should run on the ST async thread.
Bridging code for existing LSP-* plugins
We made sure that all AbstractPlugin and LspPlugin related (class)methods ran on the ST async thread. I want to now make sure all these (class)methods run on the sublime_aio thread with this pull request.
Certain methods may also be marked
asyncfor LspPlugin, most notablyon_pre_startand perhapson_initialize.The
Promiseobject can be awaited, so older AbstractPlugin/LspPlugin-related functionality returning promises from request handlers work.